-
Notifications
You must be signed in to change notification settings - Fork 69
Integration test improvements #453
Integration test improvements #453
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great! some of those definitely have been annoying. 🙇
(re)Based on #456 and while I tried to get appveyor to work, but it seems like its a deeper-rooted issue (with dat or nodejs) that I couldn't get fixed. ref. nodejs/help#1083 and nodejs/node#18391 |
Wow, rough :| |
I think it's still worth merging as is |
gotcha! can you look at the conflicts? then +1 to merging |
…l the test if the text happens to show something different in-between (like the loading bytes)
rebased on master, not sure what the problem might have been?! |
Awesome, thank you! |
Would it be possible to merge this branch with a merge commit? To keep the commit log? |
doesn't it still have the commit log in the description? d8ef48a Or do you want to see more in the history, then I'm ok with that too :) |
I asked for it in order to be able to blame. I understand the current way and I am on the fence about which one is better |
Have thought about this a little more (shoulda done that before commenting). The current statement is fine. I will keep the branch alive in order someone wants to reference it. |
Ah, I see what you're saying! Yeah I think just using git a merge commit would have been better, but with the extra information GitHub provides this gives a good balance between brevity and inspectability |
Smaller commits in future 😄 |
When trying to fix the tests for #450 I noticed some problems with how the test system is setup. In order to not bloat #450 let me push here a separate PR.